[문자열 계산기] 김연서 미션 제출합니다. by daeGULLL · Pull Request #7 · Java-JavaScript-Language-Stuty/java-calculator-7

daeGULLL · 2025-02-07T14:24:09Z

신경써서 구현한 부분

MVC 패턴과 독립성을 신경써서 구현해봤습니다!

피드백이 필요한 부분

피드백 떠오르시는거 전부 해주십쇼...

고려한 예외

음수 예외하려다가 조건이 양수라 안했습니다! 커스텀 구분자가 문자열이어도 되게끔 구현했습니다!

created feature list

Console's .read() & .print() : mapped UTC and java's method

normalDetect(char word, int index, int len) specialDetect(char word, int index, int len)

stringToInt(String arg) intToString(int num) : is utility class

consisted of input, process, output DetectResult for data handling

completed, error catch for exiting

moongua404

코드가 조금 복잡해서 제가 이해하지 못한 상태로 리뷰를 달았을 수 있었다는 점 미리 사과드립니다...ㅎㅎ 1주일이라는 시간이 많은 시간은 아니었을텐데 다양한 시도를 하신 것 같아요!
1주차 미션 수고 많으셨습니다!

moongua404 · 2025-02-08T06:55:25Z

@@ -0,0 +1,27 @@
+package calculator.Model;
+
+public class DetectResult {


모델의 책임을 작게 잡으신 것 같아요...! 모델보다는 dto에 가까운 것 같아요!

맞습니다! 솔직히 모델의 개념을 완벽히 이해하지 못해서 긴가민가하다 한 블로그 게시물에서 DTO형식대로 써뒀기에 일단 그대로 해뒀습니다...혹시 모델에 대해 추천해주실만한 글 있을까용? @moongua404

딱 맞는 글은 찾지 못했고... MVC 패턴에 대한 개괄적인 설명을 담은 글은 찾았습니다
MVC은 데이터를 담당하는 Model과 사용자와의 인터페이스 역할을 하는 View, 그 둘을 중계하는 Controller로 구성되어있습니다. 그 외에도 컨트롤러에 비지니스 로직이 섞이는 것을 막기 위해 Service, 기타 상수 관리, 검증 등의 잡무(?)를 맡는 Utility 등을 두기도 하는 것 같습니다. MVC의 핵심은 데이터의 로직을 Model이 처리하고 Controller는 최소한의 연결 역할만을 갖는다는 것 같습니다!
저 역시 디자인 패턴에 대해서는 잘 안다고 자부할 수 없기에 참고만 해 주세요

moongua404 · 2025-02-08T07:06:51Z

@@ -0,0 +1,11 @@
+package calculator.view;
+
+public class Console {


뷰도 책임이 적은 것 같아요, 좀 더 역할을 부여해도 좋을 것 같아요

오호 혹시 역할의 예시를 조금 알려주실수 있을까요?? @moongua404

print에서 인수로 받은 메시지를 출력하는 것 보다는 printGuide, printResult로 안내 및 결과 출력을 담당하도록 만드는게 좋을 것 같습니다. 이렇게 되면 입출력의 관심사가 View로 온전히 분리될 수 있기 때문입니다.
사람에 따라 다른 것 같긴 한데 read()라는 입력 함수에도 안내 설명 등의 출력을 포함시키기도 한다고 알고 있습니다.

moongua404 · 2025-02-08T07:11:59Z

+        separatorRecognizeService = new SeparatorRecognizeService();
+    }
+
+    public void input(){


이 부분은 Controller의 역할 보다는 View의 역할에 가까운 것 같아요

저도 그렇게 생각했습니다..그런데 view는 기능을 가진 채 존재만 하고 뭔가 실제로 조작할때는 컨트롤러를 사용한다고 글을 읽어서 고민하다 컨트롤러 내부에 해당 함수를 만들었었습니다..! view 이놈도 뭔가 조작? 기능을 가져도 되는건가요? 인터넷에 올라온 글들만 봐선 MVC 패턴을 잘 이해를 못하겠드라구요ㅠㅠ @moongua404

뷰에 비지니스로직이 포함되면 안되는 것은 맞지만 자료형 변환, 때로는 최소한의 타입 검사 등을 포함시키는 경우도 있는 것으로 알고있습니다.
제가 이 코드를 해석하기에는 입출력 장치만을 가져와 console.print("덧셈할 문자열을 입력해 주세요.");, rawArg = console.read(); 등 입출력의 역할을 Controller가 가져간 것 같다고 이해했습니다

moongua404 · 2025-02-08T07:14:14Z

+    private List<Integer> numberList = new ArrayList<>();
+    private int calculationResult;
+    private DetectResult[] detectResult = new DetectResult[2];
+    private char temp;


Controller의 필드가 많고, 그 중 연산 중간에 필요한 필드가 꽤 있는 것 같아요.
함수를 분리하는 등 불필요한 필드를 줄이면 가독성이 높아질 것 같아요

오호 참고하겠습니다

moongua404 · 2025-02-08T07:21:01Z

+    private boolean normalDetected=false;
+    private int specialDetected=0;
+    private int specialState = 0;
+    private int specialSeparatorDetected=0;


이쪽도 필드의 역할이 연산에 필요한 상태 관리인 것 같은데 서비스의 필드로 둘 필요는 없을 것 같아요

그 그러면 어디 필드로 두시는걸 추천하시나요? 상태관리 클래스로 분리하는게 나을까요? @moongua404

normalDetected 등의 필드가 이 서비스가 갖는 속성이라고 보기는 어려울 것 같아서 이러한 상태관리는 함수 내에서 처리하는게 좋을 것 같습니다.

moongua404 · 2025-02-08T07:29:56Z

+        return isDetected;
+    }
+
+    public void setDetected(int num){


숫자로 상태를 정하니까 코드를 쓰지 않은 사람은 상태를 이해하는게 힘들 것 같아요, 만약 상태를 이런식으로 설정해야 한다면 Enum 타입을 활용하면 좀 더 가독성이 좋아질 것 같아요

안그래도 고민하며 int쓰면 간결한걸 굳이 늘려버리는거 아닐까 생각해 넘어갔었는데 가독성이 많이 떨어지나보군요..ㅋㅋㅠ 수정하겠습니다~~

moongua404 · 2025-02-08T07:45:04Z

+
+    private SeparatorRecognizeService separatorRecognizeService;
+
+    private static String rawArg;


변수 이름을 쓸 때 축약하지 않고 좀 더 직관적인 이름이면 좋을 것 같아요! 개인적인 생각에는 inputLine 정도만 되어도 좀 더 직관적으로 이해할 수 있을 것 같아요

변수명을 짓는 건 항상 난제이지만 처음부터 너무 복잡하게 생각하지 않길 바라요!

이건 제 능지이슈로 더 최선의 것을 떠올려내지 못했었습니다...지어주신 이름 잘 쓰도록 하겠습니다 감사합니다~~~

moongua404 · 2025-02-08T09:57:51Z

+            }
+        }
+        calculationResult = NumberProcessService.addAll(numberList);
+    }


코드보다 순서논리회로(?)를 보는 것 같아요...

의존성이 개념적으로 분리되지 않은 것 같습니다. 가령 여기서 동작 원리를 이해하려면 detectResult의 getDetected를 이해해야하는데 이걸 이해하려면 normalDetect 등의 메서드를 봐야하고,,,

역할도 분리가 명확히 되지 않은 것 같습니다. process에 복잡한 내부 로직이 연산자 추출, 연산 등 다양한 역할이 섞여있는 것 같습니다

API로 대체 가능한 기능들이 꽤나 많습니다.

휴먼 에러가 발생하기 쉽울 것 같습니다. 인덱스를 통해 접근하는 것 부터 각 부분이 어떠한 기능을 하는지 파악하기 어려워 다른 사람이 코드를 다룰 때 실수가 많이 발생할 수 있을 것 같습니다.

확실히 약간 파일들끼리 꼬리의 꼬리를 무는 형태처럼 보여서 코드를 읽는 것 뿐만 아니라 작성하는데도 자세히, 조심히 봐야 할 것 같아요...

이때부터 시간이 촉박해 생각나는대로 그대로 쓰다보니 슨서논리회로가 되버린게 맞을겁니다 허허 API를 좀더 알아봐 잘 대체하고 잘 분리시켜보도록 하겠습니당

moongua404 · 2025-02-08T09:58:22Z

+
+import java.util.List;
+
+public class NumberProcessService {


서비스를 static call하는 이유가 있을까요?

Utility class를 만들때는 굳이 인스턴스 생성이 필요없어 스태틱으로 하는 것이 좋다고 전에 글에서 읽어서 써봤습니당~~

Service는 일반적으로 MVC 패턴에서 비지니스 로직을 담당하는 계층을 의미한다고 알고있습니다! 내용을 보니까 Utility처럼 쓰여서 코드의 내용으로는 static이 더 적절한 것 같네요...ㅎㅎ

아하 그럼 유틸로 분리하겠습니다~~

moongua404 · 2025-02-08T10:00:18Z

+        }
+        detectResult.setDetected(0);
+        return detectResult;
+    }


위에 함수도 그렇고 이 함수도 너무 길이가 긴 것 같아요! 코드의 흐름이 한 눈에 보이지 않을 뿐더러 여러 책임을 한 함수가 모두 지고있는 것 같아서 분리하는 쪽이 더 좋을 것 같습니다.

Regyung

코드 파일 구성에서 배울 점이 너무 많았던 PR이었습니다.

Regyung · 2025-02-09T14:39:48Z

+tasks.withType(JavaCompile) {
+    options.encoding = 'UTF-8'
+}
+


한글이 깨져서 골치인데 혹시 해당 코드로 혹시 문제가 해결 되었나요..?

넵!!! 저도 그 골치때문에 인터넷에 검색했더니 저게 답인 것 같더라구요 저거 이후에 테스트케이스 함수가 한국어라 생기던 오류가 잡혔습니다~
++엇 그런데 난슬님이 여기 수정하면 안된다 해서 gradle의 다른 파일을 만지는 방향으로 가야할 것 같습니다 저도 그렇게 수정할 예정입니다 @Regyung

Regyung · 2025-02-09T15:04:03Z

+            }
+        }
+        calculationResult = NumberProcessService.addAll(numberList);
+    }


확실히 약간 파일들끼리 꼬리의 꼬리를 무는 형태처럼 보여서 코드를 읽는 것 뿐만 아니라 작성하는데도 자세히, 조심히 봐야 할 것 같아요...

Regyung · 2025-02-09T15:05:26Z

+        return detectResult;
+    }
+}
+


해당 코드들을 정규표현식을 활용해서 구현한다면 길이를 확 줄일 수 있어 보이고 더 효율적일 것 같습니다!

맞아요 정규표현식 얘기가 이번 과제 통틀어서 많더라구요 한번 공부해서 줄여보겠습니다 감사합니다!!

Regyung · 2025-02-09T15:06:54Z

+
+    private SeparatorRecognizeService separatorRecognizeService;
+
+    private static String rawArg;


변수명을 짓는 건 항상 난제이지만 처음부터 너무 복잡하게 생각하지 않길 바라요!

seulnan

너무 고생하셨습니다 :) 전체적으로 많이 공부하신게 티가 나는 코드인 것 같아요👏🏻

추가로 첨언하자면, 리드미나 커밋을 좀 더 자세히 작성해주시면 나중에 연서님이나 다른 분들이 보기에도 상황에 대한 이해가 빨라질 것 같아요!

seulnan · 2025-02-09T14:59:23Z

+import calculator.view.Console;
+
+import java.util.ArrayList;
+import java.util.Arrays;


코드를 보니 사용되지 않는 import가 남아있는 것 같아 제거하면 좋을 것 같아요!

헉 수정할때 잘 삭제를 안하다보니 남았나보네요 삭제하겠습니당~~!!

seulnan · 2025-02-09T15:05:52Z

    }
 }

+tasks.withType(JavaCompile) {


프로그래밍요구사항
프로그래밍요구사항에 build.gradle 파일은 변경할 수 없다고 나와있습니다:)
gradle.properties파일이나 IDE 설정을 변경하는 방법으로 변경하는 것이 좋을 것 같습니다

엄마야 까먹었었네요 알려주셔서 감사합니다~~

seulnan · 2025-02-09T15:09:10Z

+
+public class NumberProcessService {
+    public static int stringToInt(String arg){
+        if(!arg.matches("\\d+")) throw new IllegalArgumentException("Invalid number format");


이 메소드에서 입력값이 null인경우의 예외처리를 더 명확하게 하는것은 어떨까요??
if (arg == null || !arg.matches("\\d+") 이런식으로요!

오 반영하겠습니다~~

seulnan · 2025-02-09T15:16:02Z

+        if(Objects.equals(rawArg, ""))calculationResult=0;
+    }
+
+    public void process(){


numberList.add(num) 이 반복적으로 호출되는 점

getDetected 값이 1일때 숫자를 리스트에 추가하는 로직이 반복되는 점
같은 중복코드가 좀 있는 것 같아요!

-> detectResult를 검사하고 숫자를 리스트에 추가하는 공통 메서드를 추출하고 getDetected 값은 한번만 검사하도록 수정해보는 방향은 어떨까요??

안그래도 그게 제 목표입니당 제가 짜놓은 로직에 따라서 그게 될지 저도 잘 모르겠지만 꿀팁 잘 받아서 해당 방향으로 한번 시도해보도록 하겠습니다~~

BYEONGHWALEE-dev · 2025-02-10T13:46:15Z

한창 배워가는 입장이라, 자세한 코드 리뷰를 달기에 힘듦이 있네요. 덕분에 MVC 모델이 무엇인지 전체적인 흐름을 알 수 있었습니다.
코드 리뷰는 아니지만, Controller와 Service의 차이가 뭔지 알 수 있을까요?

daeGULLL · 2025-02-13T12:21:03Z

@BYEONGHWALEE-dev
제가 자바 학습 정리글에 MVC패턴에 대해 상세히 정리해놓은 블로그글을 올려뒀습니다~~ 거기 보시면 잘 설명이 되어있을거에요!!

- firmed MVC pattern : implemented observer, amended model and service - build.gradle modify -> gradle.properties modify - used regex

added arg == null according to feedback

daeGULLL added 9 commits February 6, 2025 03:51

docs(README.md): initializing

2859d21

created feature list

feat($main): created Console(MVC's view)

312fa46

Console's .read() & .print() : mapped UTC and java's method

feat($main): created SeparatorRecognizeService

160a595

normalDetect(char word, int index, int len) specialDetect(char word, int index, int len)

feat($main): created NumberProcessService

4523ec6

stringToInt(String arg) intToString(int num) : is utility class

feat($main): created CalculatorController

b639ccf

consisted of input, process, output DetectResult for data handling

feat($main): completed Application

417a24b

completed, error catch for exiting

docs($README.md): added some contents

23fa090

fix($main): testcase error fixed

11a8897

fix($main): testcase error fixed

fc173b0

moongua404 reviewed Feb 8, 2025

View reviewed changes

Regyung reviewed Feb 9, 2025

View reviewed changes

seulnan reviewed Feb 9, 2025

View reviewed changes

daeGULLL added 2 commits February 14, 2025 04:22

fix($main): error fixed&refactored

0b6c837

- firmed MVC pattern : implemented observer, amended model and service - build.gradle modify -> gradle.properties modify - used regex

fix($main): NumberProcessUtil clarifying

1e3b3a5

added arg == null according to feedback

		@@ -0,0 +1,27 @@
		package calculator.Model;

		public class DetectResult {

		@@ -0,0 +1,11 @@
		package calculator.view;

		public class Console {


		private SeparatorRecognizeService separatorRecognizeService;

		private static String rawArg;

Uh oh!

Conversation

daeGULLL commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

신경써서 구현한 부분

피드백이 필요한 부분

고려한 예외

Uh oh!

moongua404 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daeGULLL Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daeGULLL Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moongua404 Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daeGULLL Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Regyung left a comment

Choose a reason for hiding this comment

Uh oh!

daeGULLL commented Feb 7, 2025 •

edited

Loading

daeGULLL Feb 9, 2025 •

edited

Loading

daeGULLL Feb 9, 2025 •

edited

Loading

moongua404 Feb 8, 2025 •

edited

Loading

daeGULLL Feb 9, 2025 •

edited

Loading

daeGULLL Feb 9, 2025 •

edited

Loading